Conversation
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
Looks good, beside the minimum number of collators
| let last_block = <LastAuthoredBlock<T>>::get(c.who.clone()); | ||
| let since_last = now.saturating_sub(last_block); | ||
| if since_last < kick_threshold { | ||
| if since_last < kick_threshold || Self::candidates().len() <= T::MinCandidates::get() { |
There was a problem hiding this comment.
| if since_last < kick_threshold || Self::candidates().len() <= T::MinCandidates::get() { | |
| if since_last < kick_threshold || Self::candidates().len() <= T::MinCandidates::get() { |
Why are you reading candidates here but also getting candidates from the input params of this function?
There was a problem hiding this comment.
so the candidates input does not get changed, it gets at the end collected into new_candidates, so if 5 people get removed, it does not know that, however the global candidates is tracking this, so it would for each candidate have the current knowledge of how many are left after the previous have been kicked
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
|
|
||
| /// Minimum number of candidates that we should have. This is used for disaster recovery. | ||
| /// | ||
| /// This does not take into account the invulnerables. |
There was a problem hiding this comment.
You are saying this count is the number of people required not including any existing invulnerables?
I think that is over doing it. Invulnerables existing should satisfy the requirement for a minimum number of collators.
There was a problem hiding this comment.
I agree in theory but invulnerables can be removed all at once (through a permissioned call) I was thinking this would be more of a assume invulnerables would be removed soon thing would be an easier transition, but open to changing
There was a problem hiding this comment.
Invulnerables should eventually be removed and by then minimum number of collators will be super useful
| pub fn leave_intent(origin: OriginFor<T>) -> DispatchResultWithPostInfo { | ||
| let who = ensure_signed(origin)?; | ||
|
|
||
| ensure!(Self::candidates().len() as u32 > T::MinCandidates::get(), Error::<T>::TooFewCandidates); |
There was a problem hiding this comment.
shouldn't this logic be in try_remove_candidate?
Else it may not be checked everywhere that this is happening correctly.
There was a problem hiding this comment.
I had originally looked at that, but try_remove_candidates is used in kick_candidates and that would track two items, the candidates array and the current candidates to be accumulated as collators. So if one failed while the other passed they would not track each other correctly. So it was either rewrite the function, or add this here, this was less code and I don't see how this would not cause it to track properly.
* min collator check * change statemint/mine min candidates * Ci pass * Update pallets/collator-selection/src/lib.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * Update pallets/collator-selection/src/lib.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * Apply suggestions from code review * build fixes * add error messages to errors * added validator register check Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
* min collator check * change statemint/mine min candidates * Ci pass * Update pallets/collator-selection/src/lib.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * Update pallets/collator-selection/src/lib.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> * Apply suggestions from code review * build fixes * add error messages to errors * added validator register check Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
4 participants
This PR checks to make sure that there is a min amount of collators before a collator gets kicked, or tries to leave. This is a defence mechanism for better disaster recovery of a chain